Skip to content

Conversation

@mickleness
Copy link
Contributor

@mickleness mickleness commented Jun 9, 2025

There were a few action bindings available in JPasswordFields in Aqua that let you identify the boundaries of words.

This came to my attention while looking at the related work #25443 . In that PR we said we should iterate across all available L&Fs, so this PR copies that same approach. (The original complaint only focused on Aqua, though.)


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8358813: JPasswordField identifies spaces in password via delete shortcuts (Bug - P3)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25688/head:pull/25688
$ git checkout pull/25688

Update a local copy of the PR:
$ git checkout pull/25688
$ git pull https://git.openjdk.org/jdk.git pull/25688/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25688

View PR using the GUI difftool:
$ git pr show -t 25688

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25688.diff

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 9, 2025

👋 Welcome back mickleness! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Jun 9, 2025

@mickleness This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8358813: JPasswordField identifies spaces in password via delete shortcuts

Reviewed-by: aivanov, dnguyen

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 40 new commits pushed to the master branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@honkar-jdk, @aivanov-jdk, @DamonGuy) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk
Copy link

openjdk bot commented Jun 9, 2025

@mickleness The following label will be automatically applied to this pull request:

  • client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@mickleness
Copy link
Contributor Author

IMO this PR is a simple response that interfaces with an existing method. I recommend this approach because it should be relatively low-risk, and it is based on existing precedent.

By contrast: #25443 does something more aggressive (and interesting). It replaces selectWordAction with selectLineAction. That looks (to me) like a more thorough/better approach to the general problem of accidentally interacting with words in a password field. This PR should resolve any KeyStroke-based actions, but in JDK-8354646 the original complaint had to do with double-clicking the mouse; so this PR wouldn't impact those steps.

I haven't explored this thoroughly yet, but glancing through the AccessibleJTextField suggests an AX interface could still have access (via AccessibleActions) to the word-related actions we're trying to restrict access to. If so: this problem (accessing words in a password field) keeps popping up, and removing/replacing the unwanted actions seems like the most thorough response.

(Also see JDK-6191897 and JDK-4231444 ).

@openjdk openjdk bot added the rfr Pull request is ready for review label Jun 27, 2025
@mlbridge
Copy link

mlbridge bot commented Jun 27, 2025

Webrevs

Copy link
Contributor

@honkar-jdk honkar-jdk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a mismatch between the PR title and JBS title JDK-8358813.

@mickleness mickleness changed the title 8358813: remove more bindings for JPasswordFields in Aqua 8358813: JPasswordField identifies spaces in password via delete shortcuts Jun 27, 2025
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, update the copyright year.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer necessary after merging with master ( see 2bff8e0 )

Comment on lines 72 to 76
for (int condition : new int[] {
JComponent.WHEN_IN_FOCUSED_WINDOW,
JComponent.WHEN_FOCUSED,
JComponent.WHEN_ANCESTOR_OF_FOCUSED_COMPONENT
}) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The array with input map types could be a static constant just above runTest method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this is updated.

Comment on lines 84 to 93
if (actionBinding == DefaultEditorKit.deleteNextWordAction ||
actionBinding == DefaultEditorKit.deletePrevWordAction ||
actionBinding == DefaultEditorKit.beginWordAction ||
actionBinding == DefaultEditorKit.endWordAction ||
actionBinding == DefaultEditorKit.selectionBeginWordAction ||
actionBinding == DefaultEditorKit.selectionEndWordAction ||
actionBinding == DefaultEditorKit.previousWordAction ||
actionBinding == DefaultEditorKit.nextWordAction ||
actionBinding == DefaultEditorKit.selectionPreviousWordAction ||
actionBinding == DefaultEditorKit.selectionNextWordAction ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to put the actions in a list or array? Then listOfActions.contains(actionBinding) instead of this long if condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a preference. I pushed an update that wraps them in a TreeSet.

@aivanov-jdk
Copy link
Member

Client tests are green.

Copy link
Contributor

@DamonGuy DamonGuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with and without the change to AquaKeyBindings. Passes after adding null to the 3 key bindings. Ran on clientlibs tests and all tests still pass. I left a question on the bindings segment for clarification.

"shift alt KP_RIGHT", null,
"alt BACK_SPACE", null,
"ctrl W", null,
"alt DELETE", null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change "works" for the test, but I'm not exactly sure what's changed here. When I make my own local test with a JPasswordField, the key binds don't do anything on Aqua. The CTRL+W seems to just input W. ALT+BACK_SPACE seems to just backspace the character before the carat. Can you help me understand this?

Also, I guess you can end in a trailing comma to match the previous format if you want. Either is OK.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DamonGuy it sounds like maybe (?) you're not able to reproduce the original ticket?

Here's a video showing what I experience in JDK 24 vs what I see when I use this PR:
https://go.screenpal.com/watch/cTjfh4nI55H

Does this match what you're observing/experiencing, and if not: what do you see instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DamonGuy I can reproduce the problem with the test, PasswordSelectionWordTest.java, attached to JDK-8358813.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I guess you can end in a trailing comma to match the previous format if you want. Either is OK.

This is a nice suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I added a trailing comma

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the video showcase. I was catching up yesterday after being out and I can now see the issue. I can match what is shown now as well. I might have built the image with incomplete changes the first go around.

/**
* These are all the actions with "word" in their field name.
*/
static Collection<String> wordActions = new TreeSet<>(Arrays.asList(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would Set.of do the job?

Perhaps, List.of would be as good.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this is updated to Set.of

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Using Set ensures there are no duplicates in the list.

@openjdk
Copy link

openjdk bot commented Aug 6, 2025

⚠️ @mickleness the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout JDK-8358813
$ git commit --author='Preferred Full Name <[email protected]>' --allow-empty -m 'Update full name'
$ git push

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Aug 6, 2025
@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Aug 6, 2025
@openjdk openjdk bot added the ready Pull request is ready to be integrated label Aug 6, 2025
@mickleness
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Aug 6, 2025
@openjdk
Copy link

openjdk bot commented Aug 6, 2025

@mickleness
Your change (at version e618df3) is now ready to be sponsored by a Committer.

@aivanov-jdk
Copy link
Member

Client tests are green. I ran a job yesterday with the latest updates.

@aivanov-jdk
Copy link
Member

/sponsor

@openjdk
Copy link

openjdk bot commented Aug 7, 2025

Going to push as commit 8d73fe9.
Since your change was applied there have been 44 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Aug 7, 2025
@openjdk openjdk bot closed this Aug 7, 2025
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Aug 7, 2025
@openjdk
Copy link

openjdk bot commented Aug 7, 2025

@aivanov-jdk @mickleness Pushed as commit 8d73fe9.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@aivanov-jdk
Copy link
Member

@mickleness You should associate your GitHub account with your OpenJDK id, follow the instructions in Associating your GitHub account and your OpenJDK username.

After that, I also recommend following the instructions in the OpenJDK Email section.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

client [email protected] integrated Pull request has been integrated

Development

Successfully merging this pull request may close these issues.

5 participants